Ordering concern: window.location.href = url triggers a hard navigation, then throw err fires. In a real browser, throw would likely still execute because location.href assignment doesn't synchronously unload the page. But the thrown error propagates to the caller, which emits routeChangeError — is that the intended behavior?

In the old code, these were early returns, so the caller proceeded to emit routeChangeComplete (which was wrong, hence the fix). The new throw correctly prevents routeChangeComplete, but the caller now emits routeChangeError and the page is doing a hard navigation. This double-action seems intentional (the error event lets listeners know what happened before the page unloads), but it means routeChangeError listeners must be synchronous to observe the event before the hard nav triggers. Worth a brief comment explaining this contract.

When NavigationCancelledError is caught, the method returns true (success). This means calling code like const ok = await router.push('/page-a') gets true even though the navigation was cancelled and the content never loaded for /page-a.

Next.js's router.push also returns true for cancelled navigations (because the intent was replaced by a newer navigation, not because it failed), so this matches. But it might be worth a comment here explaining the rationale, since return true for a cancelled navigation is counterintuitive.

The semantics are now documented in runNavigateClient()'s return type and JSDoc — "cancelled" is explicitly distinguished from "completed" and "failed", making the caller's intent clearer than a boolean.

Nit: the if branch and the fallthrough do the exact same thing (routerEvents.emit("routeChangeError", ...)). The instanceof check here doesn't change behavior — both paths emit the same event. If the intent is to differentiate in the future (e.g., log non-cancellation errors, or attempt a hard navigation fallback for non-cancel errors), this makes sense as a structural placeholder. But as-is, it could be simplified to a single routerEvents.emit("routeChangeError", err, fullAppUrl, { shallow: false }) without the if.

Same applies to the 4 identical try/catch blocks in push/replace.

This is now moot — the instanceof check lives in runNavigateClient() once, not 4 times. The three-way return type ("completed" | "cancelled" | "failed") makes the branching explicit.

Good: aborting the previous controller before creating a new one is the correct order. If the abort handler in the previous navigation's fetch runs synchronously (some environments do this), it will see _navigationId already incremented and correctly throw NavigationCancelledError.

One edge case to consider: if navigateClient is called with the same URL (e.g., user double-clicks a link), the current navigation aborts itself. This is arguably correct (the second call takes over), but it does mean the first fetch is wasted even though it was fetching the same resource. Not a bug, just a minor inefficiency — and it matches Next.js behavior.

The test resolves fetchB before fetchA, which is the scenario where the winning navigation's response arrives first. It would also be valuable to test the reverse: resolve fetchA first (stale response arrives before the winning one). The cancel mechanism should still work because assertStillCurrent() fires after each await, but the timing is different — the stale navigation gets further through the pipeline before being killed.

Not blocking, but would strengthen confidence in the assertStillCurrent checkpoint coverage.

The mock fetch doesn't check _init.signal — it ignores the AbortController's signal entirely. This means the tests don't actually verify that the AbortController integration works (i.e., that a real fetch would throw AbortError when the signal fires). The deferred pattern tests the navigation ID mechanism, but the abort signal path is untested.

Consider adding a test where the mock fetch respects the signal:

navigateClient() no longer has a catch that falls back to hard navigation on unexpected failures. Several operations here can still throw (e.g. JSON.parse(match[1]), dynamic import(pageModuleUrl), import('react'), root.render), but in those cases the caller currently only emits routeChangeError and returns false — leaving the URL already updated via pushState/replaceState/popstate while the old content stays rendered. Consider reintroducing a catch in navigateClient (or adding equivalent fallback in every caller) that, for non-cancel errors, sets window.location.href = url before rethrowing so the browser reliably lands on the correct page.

Fixed. runNavigateClient() now does window.location.href = fullUrl for non-cancel errors, restoring the hard-navigation recovery that the old code had.

These tests attach Router.events handlers but never detach them. Since routerEvents is a module-level singleton and the module is cached across tests, listeners from earlier cases will accumulate and continue firing in later tests (and across the whole shims test suite), which can cause leaks/flakiness. Suggest storing the handler functions and calling Router.events.off(...) in the test finally blocks, or using vi.resetModules() + fresh import per test to isolate the event emitter.

These listeners are already cleaned up in the finally block — see lines 8606-8608 where Router.events.off("routeChangeComplete", onRouteChangeComplete) runs. All 7 tests have equivalent cleanup.

This Router.events.on("routeChangeError", ...) handler is never removed. Because Router.events is a module-level singleton, handlers will accumulate across test cases when the module is cached. Please call Router.events.off(...) in finally (or reset modules) to keep tests isolated and avoid leaks.

These Router.events listeners are never detached. With a cached router module, they will accumulate across tests and continue firing later, which can lead to leaks/flakiness. Prefer removing them in finally via Router.events.off(...) (or vi.resetModules() per test).

These Router.events listeners are never removed. Since routerEvents is a singleton in the cached router module, listeners will stack up across tests; please detach them in finally with Router.events.off(...) (or reset modules between tests).

This Router.events.on("routeChangeError", ...) handler is not removed, so it will persist into later tests when the router module is cached. Suggest calling Router.events.off(...) in finally (or using vi.resetModules()) to prevent listener accumulation.

This Router.events.on("routeChangeComplete", ...) handler is never detached. Because Router.events is a singleton, the handler will persist and fire in later tests. Consider removing it in finally (Router.events.off) or resetting the router module between tests.

Nit: consider exporting NavigationCancelledError (or at least the type) so consumers can distinguish cancellations from real errors in routeChangeError listeners without relying on duck-typing .cancelled. Next.js doesn't export a named class for this, so duck-typing is the documented pattern — but exporting the class would be a nice DX improvement.

Not blocking.

Agreed this would be nice DX. Keeping duck-typing for now to match Next.js's convention — consumers already check .cancelled. Can revisit as a follow-up if there's demand.

Bug (low-severity): window.__NEXT_DATA__ is written on line 444 (below), between this assertStillCurrent() and the next one on line 472. Everything between lines 433–471 is synchronous so there's no microtask interleaving within this function, but the issue is:

1. Stale navigation A's fetch resolves → passes assertStillCurrent() here
2. Synchronously writes window.__NEXT_DATA__ = nextDataA (line 444)
3. Hits await import(pageModuleUrl) on line 471 — yields to microtask queue
4. Winning navigation B's microtasks run, eventually overwriting __NEXT_DATA__

Between steps 2 and 4, __NEXT_DATA__ holds stale data. Any code reading it during that window (analytics, routeChangeError listeners from the stale nav's cancellation on line 472) sees inconsistent state.

Fix: Defer window.__NEXT_DATA__ = nextData until just before root.render(element) (around line 516), after all assertStillCurrent() checks have passed. Keep const nextData = JSON.parse(...) and const { pageProps } = nextData.props where they are — just delay the global write.

Fixed. Deferred the window.__NEXT_DATA__ write to just before root.render(), after all assertStillCurrent() checkpoints have passed. Added a regression test that asserts __NEXT_DATA__ does not reflect the cancelled route's data at the moment routeChangeError fires.

Regression: The popstate rejection handler emits routeChangeError but doesn't fall back to hard navigation for non-cancellation errors.

The old code had window.location.href = url inside navigateClient()'s catch block, which recovered from unexpected failures (JSON parse errors, import failures, etc.) by doing a full page load. With this PR, those errors propagate out as throws. The 4 push/replace call sites catch them and return false, but the popstate handler only emits the event and stops — leaving the browser showing the popstate target URL with the old rendered content.

Consider adding a hard-navigation fallback for non-cancel errors here:

(err: unknown) => {
  routerEvents.emit("routeChangeError", err, fullAppUrl, { shallow: false });
  if (!(err instanceof NavigationCancelledError)) {
    window.location.href = browserUrl;
  }
},

The same gap exists in the 4 push/replace catch blocks — they return false but don't trigger a hard navigation, so the user is stuck on the old content with a new URL. In the old code, navigateClient handled this internally. Might be worth restoring that fallback inside navigateClient's own error path (for non-cancel errors) rather than relying on every caller to do it.

Fixed. The popstate rejection handler now does window.location.href = browserUrl for non-cancel errors. Same fallback is in the new runNavigateClient() helper used by push/replace.

This identical 12-line try/catch block is duplicated 4 times: useRouter().push (here), useRouter().replace (line 701), Router.push (line 900), and Router.replace (line 961).

Consider extracting a helper:

async function runNavigateClient(
  full: string,
  resolved: string,
): Promise<{ ok: true } | { ok: false; cancelled: boolean }> {
  try {
    await navigateClient(full);
    return { ok: true };
  } catch (err: unknown) {
    routerEvents.emit("routeChangeError", err, resolved, { shallow: false });
    if (!(err instanceof NavigationCancelledError)) {
      window.location.href = full; // hard-navigation fallback
    }
    return { ok: false, cancelled: err instanceof NavigationCancelledError };
  }
}

This also gives a natural place to add the hard-navigation fallback for non-cancel errors (addressing point 2 above).

Not blocking, but strongly recommended before this file grows further.

Done. Extracted runNavigateClient() returning "completed" | "cancelled" | "failed" so callers can distinguish all three outcomes. The 4 push/replace sites are now 3 lines each instead of 12.

Good test. One subtlety: this assertion passes even with the early-write issue on router.ts:444, because navigation B completes last and overwrites A's __NEXT_DATA__. To catch the early-write bug, you'd need to observe __NEXT_DATA__ at the moment navigation A is cancelled.

Consider adding an assertion inside an error handler:

const onError = (...args: unknown[]) => {
  // At the moment of cancellation, __NEXT_DATA__ should NOT reflect the cancelled route
  expect(win.__NEXT_DATA__.page).not.toBe("/page-a");
};
Router.events.on("routeChangeError", onError);

This would fail today (demonstrating the early-write issue) and would pass after deferring the __NEXT_DATA__ assignment. Not blocking for this PR, but would be valuable to add alongside the fix.

Added this test — it asserts inside a routeChangeError handler that __NEXT_DATA__.page is not the cancelled route's value. Passes now that the write is deferred.

Good comment explaining the window.location.href + throw interaction. One small addition worth noting: the callers catch this error and return false, but they don't prevent the hard navigation already scheduled by window.location.href = url. So the caller gets false AND a hard navigation happens. This is correct (caller learns it failed, browser recovers), but callers should not attempt their own hard-navigation fallback since one is already in flight.

Worth adding a note like: // Callers must NOT schedule a second hard navigation — this assignment already queues one.

Added a note: Callers (runNavigateClient) must NOT schedule a second hard navigation — this assignment already queues one.

Bug (low severity): This line sets window.location.href = fullUrl for every non-cancel error. But navigateClient() already sets window.location.href = url for all its known failure modes (non-OK response at line 428, missing __NEXT_DATA__ at line 439, no module URL at line 461, invalid path at line 469, no default export at line 480) before throwing.

This means known failures trigger two window.location.href assignments. The second is a no-op in practice, but it contradicts the comment on line 427 which explicitly says callers must NOT schedule a second hard navigation.

The window.location.href fallback here is still valuable for unexpected errors (e.g., JSON.parse throws, import() rejects with a non-abort error) where navigateClient didn't schedule a hard nav. But as written, it fires unconditionally.

Consider guarding it:

Or, longer term, consolidate all hard-nav assignments here and remove them from navigateClient() — let it be a pure "try or throw" function.

Good: the closure captures navId by value, so each navigation has a stable snapshot to compare against. The inner function is also nicely minimal — no allocation, just an integer comparison and throw.

One thought: if you ever need to expose cancellation status to external code (e.g., for testing or debugging), consider returning a boolean from assertStillCurrent() instead of throwing, and let the caller decide whether to throw. But for now the throw pattern is the right call — it's simpler and matches the Next.js clc pattern.

Subtle but important: there is no assertStillCurrent() between the last checkpoint (line 505, after _app import) and this window.__NEXT_DATA__ write + root.render(). The gap covers:

1. React.createElement(AppComponent, ...) or React.createElement(PageComponent, ...)
2. wrapWithRouterContext(element)
3. window.__NEXT_DATA__ = nextData
4. root.render(element)

All of these are synchronous, so there's no microtask interleaving between them — a newer navigation can't start between lines 507 and 523. This is fine today, but it's a fragile invariant. If wrapWithRouterContext or element construction ever becomes async (e.g., lazy component resolution), the gap would become a real bug.

Worth a brief comment noting the synchronous invariant:

The first test doesn't clean up event listeners because it doesn't attach any — good. But it also doesn't reset the module-level _navigationId and _activeAbortController between tests. Since the router module is cached via import(), these module-level variables persist across all tests in the describe block.

This isn't a bug today because the monotonic counter works correctly regardless of its starting value, and the abort controller is cleaned up in navigateClient's finally block. But if a test ever leaves a navigation in-flight (e.g., by not awaiting Promise.allSettled), the stale controller could interfere with the next test.

The current tests all properly await Promise.allSettled, so this is fine — just noting the coupling for future test authors.

Nit: missing blank line separating this test from the previous one (all other tests in the block have one). Trivial.

The test name says "schedule a single hard navigation" but the assertion expects 2 href assignments. This is documenting the double-write (once inside scheduleHardNavigationAndThrow, once in runNavigateClient's catch) rather than asserting against it.

Either:

1. Fix the code so known failures only assign window.location.href once (the HardNavigationScheduledError guard in runNavigateClient already handles this — the double-write only happens because scheduleHardNavigationAndThrow throws HardNavigationScheduledError which is correctly caught, so runNavigateClient skips the second assignment... wait, looking again at line 571: !(err instanceof HardNavigationScheduledError) — this guard does prevent the double write. So why does the test expect 2?)
2. Or update the test name to reflect what it actually verifies.

I suspect the test is seeing 2 assignments because of pushState's mock updating win.location.href (the trackHrefAssignments getter/setter intercepts that too). Worth debugging — if the guard is working correctly, there should only be 1 scheduleHardNavigationAndThrow assignment for /failing-page plus 0 from runNavigateClient. The second may be coming from the pushState mock.

The popstate handler calls navigateClient() directly instead of runNavigateClient(). This means the popstate error path:

1. Doesn't benefit from the HardNavigationScheduledError guard — known failures (non-OK response, missing __NEXT_DATA__) will assign window.location.href twice (once in scheduleHardNavigationAndThrow, once on line 866).
2. Duplicates the error-handling logic that runNavigateClient already centralizes.